feat(start): add pre-render params + sitemap#7346
Open
nikuscs wants to merge 12 commits intoTanStack:mainfrom
Open
feat(start): add pre-render params + sitemap#7346nikuscs wants to merge 12 commits intoTanStack:mainfrom
nikuscs wants to merge 12 commits intoTanStack:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds typed ChangesTyped Dynamic Route Prerendering
Sequence Diagram(s)sequenceDiagram
participant CLI as Build CLI
participant Bundler as Vite/RSBuild
participant ServerBundle as Built Server/Prerender Bundle
participant PrerenderRunner as runPrerenderParams
participant Files as Output (dist) / Sitemap
CLI->>Bundler: build (with separatePrerenderRouteOptions?)
Bundler->>ServerBundle: emit server/prerender bundles
CLI->>ServerBundle: (prerender) load route-options entry (import with cache-bust)
ServerBundle->>PrerenderRunner: provide routeTree & prerenderParams callbacks
PrerenderRunner->>PrerenderRunner: run callbacks (with AbortSignal/timeouts)
PrerenderRunner->>Files: write generated pages and merged sitemap entries
PrerenderRunner->>Bundler: ensure client bundles exclude server-only markers
PrerenderRunner->>CLI: return pages list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
github-actions
Bot
added
documentation
Contributor
|
View your CI Pipeline Execution ↗ for commit 2cba39a
☁️ Nx Cloud last updated this comment at |
Contributor
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/vue-start/basic/rsbuild.config.ts (1)
8-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
sitemapoption to rsbuild prerender config for feature parity with viteThe Vue
vite.config.tsconditionally enablessitemapgeneration whenisPrerenderis true, butrsbuild.config.tsomits this option. The prerendering test includes sitemap assertions, but usestest.skip()to gracefully skip the test if the sitemap is missing — so this won't cause a test failure. However, it creates an inconsistency where users building with rsbuild won't getsitemap.xmloutput during prerendering, unlike the vite build.🛠️ Proposed fix (mirror the vite config)
const prerenderConfiguration = { ... } export default defineConfig({ plugins: [ pluginBabel({ include: /\.(?:jsx|tsx)$/ }), pluginVue(), pluginVueJsx(), - tanstackStart({ - prerender: isPrerender ? prerenderConfiguration : undefined, - }), + tanstackStart({ + prerender: isPrerender ? prerenderConfiguration : undefined, + sitemap: isPrerender + ? { enabled: true, host: 'https://example.com' } + : undefined, + }), ],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/vue-start/basic/rsbuild.config.ts` around lines 8 - 37, The rsbuild prerender config omits the sitemap option so rsbuild builds won't produce sitemap.xml; update the prerender configuration passed to tanstackStart in rsbuild.config.ts by adding sitemap: true when prerendering is enabled (e.g., extend prerenderConfiguration or the object passed to tanstackStart to include sitemap: true when isPrerender is true), referencing prerenderConfiguration, tanstackStart and isPrerender so the rsbuild prerender behavior matches the vite config.
🧹 Nitpick comments (5)
e2e/solid-start/basic/rsbuild.config.ts (2)
7-23: 💤 Low valueDuplicate prerender filter list with
vite.config.ts— consider extracting a shared config helperThe
prerenderConfiguration(filter list +maxRedirects) defined here is byte-for-byte identical to theprerenderConfigurationine2e/solid-start/basic/vite.config.ts. The React e2e app already centralizes this ine2e/react-start/basic/start-mode-config.tsviagetStartModeConfig(). Applying the same pattern for Solid (and Vue) prevents silent filter-list drift when new routes are added.♻️ Proposed refactor — extract a shared start-mode-config.ts for Solid
Create
e2e/solid-start/basic/start-mode-config.ts:import { isPrerender } from './tests/utils/isPrerender' import { isSpaMode } from './tests/utils/isSpaMode' export function getStartModeConfig() { return { spa: isSpaMode ? { enabled: true, prerender: { outputPath: 'index.html' } } : undefined, prerender: isPrerender ? { enabled: true, filter: (page: { path: string }) => ![ '/this-route-does-not-exist', '/redirect', '/i-do-not-exist', '/not-found', '/specialChars/search', '/specialChars/hash', '/specialChars/malformed', '/search-params/default', '/transition', '/users', ].some((p) => page.path.includes(p)), maxRedirects: 100, } : undefined, sitemap: isPrerender ? { enabled: true, host: 'https://example.com' } : undefined, } }Then in both
vite.config.tsandrsbuild.config.ts:-import { isPrerender } from './tests/utils/isPrerender' +import { getStartModeConfig } from './start-mode-config' -const prerenderConfiguration = { ... } ... -tanstackStart({ prerender: isPrerender ? prerenderConfiguration : undefined, sitemap: ... }) +tanstackStart(getStartModeConfig())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/solid-start/basic/rsbuild.config.ts` around lines 7 - 23, The prerenderConfiguration object (including filter and maxRedirects) is duplicated; extract it into a shared getStartModeConfig() helper (like the React approach) that returns the prerender config (with enabled, filter function, maxRedirects and sitemap/spa variants) and import that helper into both vite.config.ts and rsbuild.config.ts, then replace the local prerenderConfiguration constant with the shared value (referencing getStartModeConfig and its prerender property) so both build configs use the same filter array and settings.
7-35: ⚡ Quick win
sitemapoption missing from rsbuild prerender config — feature parity gap
e2e/solid-start/basic/vite.config.tsincludessitemap: { enabled: true, host: 'https://example.com' }whenisPrerenderis true, but the rsbuild config does not. While the sitemap test inprerendering.spec.ts(line 147) includes atest.skip()guard that gracefully skips when the sitemap is disabled, the inconsistency means the rsbuild build lacks this feature compared to vite. Consider adding the sitemap option for parity:Optional enhancement
tanstackStart({ prerender: isPrerender ? prerenderConfiguration : undefined, + sitemap: isPrerender + ? { enabled: true, host: 'https://example.com' } + : undefined, }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/solid-start/basic/rsbuild.config.ts` around lines 7 - 35, The rsbuild config is missing the sitemap option for prerender parity; update the tanstackStart call in rsbuild.config.ts (where tanstackStart({ prerender: isPrerender ? prerenderConfiguration : undefined })) so that when isPrerender is true the prerender config includes sitemap: { enabled: true, host: 'https://example.com' } (either by adding sitemap to the existing prerenderConfiguration object or by merging it when passing into tanstackStart).packages/start-client-core/src/prerenderParams.ts (1)
61-90: ⚡ Quick winDocument expected date string formats.
lastmodandnews.publicationDateacceptstring | Date, but the sitemap writer normalizes toYYYY-MM-DD(perbuild-sitemap.test.ts). Adding short JSDoc on these fields (e.g., "ISO 8601 date orYYYY-MM-DD;Dateis formatted to W3C date format") would prevent users from passing arbitrary strings that get emitted verbatim into the XML.📝 Proposed JSDoc additions
alternateRefs?: Array<{ href: string hreflang: string }> + /** + * Last modification date. Accepts a `Date` (formatted as `YYYY-MM-DD`) + * or a W3C/ISO 8601 date string. Strings are emitted verbatim, so + * pass `YYYY-MM-DD` or full ISO 8601 to remain spec-compliant. + */ lastmod?: string | Date ... news?: { publication: { name: string language: string } + /** + * Publication date. `Date` is formatted as `YYYY-MM-DD`; + * strings should be W3C/ISO 8601 to remain spec-compliant. + */ publicationDate: string | Date title: string }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/start-client-core/src/prerenderParams.ts` around lines 61 - 90, Update the RouteSitemapOptions interface JSDoc to document expected date string formats for lastmod and news.publicationDate: add brief comments on the lastmod property and the nested news.publicationDate explaining they accept ISO 8601 strings or YYYY-MM-DD and that Date values will be formatted to W3C/ISO (normalized to YYYY-MM-DD by the sitemap writer). Reference the RouteSitemapOptions interface and its lastmod and news.publicationDate fields so consumers know strings will be emitted verbatim unless they follow the documented formats.packages/start-plugin-core/tests/prerender-routes-plugin.test.ts (1)
39-69: 💤 Low valueLGTM – consider adding a
TSS_PRERENDER_DYNAMIC_ROUTESassertion to test 2.Test 2 verifies that
/posts/$slug(which hascomponentbut notprerenderParams) stays out ofTSS_PRERENDABLE_PATHS, but doesn't confirm it's also absent fromTSS_PRERENDER_DYNAMIC_ROUTES. Test 3 covers this indirectly, so it's not a bug — just a small coverage gap.✨ Optional assertion to make test 2 self-contained
expect(globalThis.TSS_PRERENDABLE_PATHS).toEqual([{ path: '/' }]) + expect(globalThis.TSS_PRERENDER_DYNAMIC_ROUTES).toEqual([]) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/start-plugin-core/tests/prerender-routes-plugin.test.ts` around lines 39 - 69, Add an assertion in the test "does not store API, layout, or dynamic routes as static paths" to also verify that dynamic routes without prerenderParams are not recorded in TSS_PRERENDER_DYNAMIC_ROUTES: after invoking plugin.onRouteTreeChanged (using prerenderRoutesPlugin and the route node with routePath '/posts/$slug'), assert that globalThis.TSS_PRERENDER_DYNAMIC_ROUTES does not contain an entry for '/posts/$slug' (or is empty), so the test checks both TSS_PRERENDABLE_PATHS and globalThis.TSS_PRERENDER_DYNAMIC_ROUTES consistency.packages/start-plugin-core/src/vite/prerender.ts (1)
38-50: ⚡ Quick winAddress the fragile output filename assumption.
The code computes the server output filename as
${basename(serverInput, extname(serverInput))}.js, which assumes the built file uses the same base name as the input and always ends in.js. A project that setsrollupOptions.output.entryFileNameswith a hash pattern or emits.cjs/.mjswould produce anERR_MODULE_NOT_FOUNDcrash at prerender time. Consider reading from a Vite manifest or build metadata to get the actual output filename instead of computing it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/start-plugin-core/src/vite/prerender.ts` around lines 38 - 50, The code that builds serverEntryPath from serverInput (using basename + ".js") is fragile for hashed or non-.js outputs; instead look up the actual built entry filename from the Vite/Rollup manifest or build output metadata and use that to form serverEntryPath. Concretely: when preparing to import the built server entry, read the build manifest (or Rollup output info) in the same output directory returned by getServerOutputDirectory(serverEnv.config), find the entry whose source or file corresponds to serverInput (use serverInput as the original input key), and use the manifest-provided filename (which may be .cjs/.mjs or hashed) for import; update the logic around serverInput, serverOutputDir, serverEntryPath and the await import(...) to use the manifest-resolved filename instead of `${basename(...)}.js`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/vue-start/basic/tests/prerendering.spec.ts`:
- Line 119: The assertion expect(html).toContain('2') is too broad; update the
test to assert the specific element or text that indicates the search param
page=2 was rendered instead of any occurrence of the character '2'. Replace the
loose expect(html).toContain('2') check (referencing the html variable and the
expect call) with a targeted assertion—either assert the exact HTML substring
for the element that should show "2" (e.g., the specific heading or pager
markup) or parse html with a DOM parser (e.g., JSDOM) and use querySelector to
assert the element's textContent equals '2' (or matches a precise regex like
/\bpage[:\s]*2\b/).
In `@packages/start-plugin-core/src/prerender-params-runner.ts`:
- Around line 131-142: The promise executor currently checks signal.aborted once
but then schedules callback via Promise.resolve().then(callback) allowing a
race; change the scheduled invocation to first re-check signal.aborted and
throw/reject if set before calling callback (e.g. replace .then(callback) with
.then(() => { if (signal.aborted) throw signal.reason ?? new
Error('prerenderParams aborted'); return callback(); })) and ensure the abort
listener (abort) is removed/cleanup occurs after resolve/reject so the handler
and callback cannot both run; reference the Promise executor, signal, callback,
and abort variables in prerender-params-runner.ts.
---
Outside diff comments:
In `@e2e/vue-start/basic/rsbuild.config.ts`:
- Around line 8-37: The rsbuild prerender config omits the sitemap option so
rsbuild builds won't produce sitemap.xml; update the prerender configuration
passed to tanstackStart in rsbuild.config.ts by adding sitemap: true when
prerendering is enabled (e.g., extend prerenderConfiguration or the object
passed to tanstackStart to include sitemap: true when isPrerender is true),
referencing prerenderConfiguration, tanstackStart and isPrerender so the rsbuild
prerender behavior matches the vite config.
---
Nitpick comments:
In `@e2e/solid-start/basic/rsbuild.config.ts`:
- Around line 7-23: The prerenderConfiguration object (including filter and
maxRedirects) is duplicated; extract it into a shared getStartModeConfig()
helper (like the React approach) that returns the prerender config (with
enabled, filter function, maxRedirects and sitemap/spa variants) and import that
helper into both vite.config.ts and rsbuild.config.ts, then replace the local
prerenderConfiguration constant with the shared value (referencing
getStartModeConfig and its prerender property) so both build configs use the
same filter array and settings.
- Around line 7-35: The rsbuild config is missing the sitemap option for
prerender parity; update the tanstackStart call in rsbuild.config.ts (where
tanstackStart({ prerender: isPrerender ? prerenderConfiguration : undefined }))
so that when isPrerender is true the prerender config includes sitemap: {
enabled: true, host: 'https://example.com' } (either by adding sitemap to the
existing prerenderConfiguration object or by merging it when passing into
tanstackStart).
In `@packages/start-client-core/src/prerenderParams.ts`:
- Around line 61-90: Update the RouteSitemapOptions interface JSDoc to document
expected date string formats for lastmod and news.publicationDate: add brief
comments on the lastmod property and the nested news.publicationDate explaining
they accept ISO 8601 strings or YYYY-MM-DD and that Date values will be
formatted to W3C/ISO (normalized to YYYY-MM-DD by the sitemap writer). Reference
the RouteSitemapOptions interface and its lastmod and news.publicationDate
fields so consumers know strings will be emitted verbatim unless they follow the
documented formats.
In `@packages/start-plugin-core/src/vite/prerender.ts`:
- Around line 38-50: The code that builds serverEntryPath from serverInput
(using basename + ".js") is fragile for hashed or non-.js outputs; instead look
up the actual built entry filename from the Vite/Rollup manifest or build output
metadata and use that to form serverEntryPath. Concretely: when preparing to
import the built server entry, read the build manifest (or Rollup output info)
in the same output directory returned by
getServerOutputDirectory(serverEnv.config), find the entry whose source or file
corresponds to serverInput (use serverInput as the original input key), and use
the manifest-provided filename (which may be .cjs/.mjs or hashed) for import;
update the logic around serverInput, serverOutputDir, serverEntryPath and the
await import(...) to use the manifest-resolved filename instead of
`${basename(...)}.js`.
In `@packages/start-plugin-core/tests/prerender-routes-plugin.test.ts`:
- Around line 39-69: Add an assertion in the test "does not store API, layout,
or dynamic routes as static paths" to also verify that dynamic routes without
prerenderParams are not recorded in TSS_PRERENDER_DYNAMIC_ROUTES: after invoking
plugin.onRouteTreeChanged (using prerenderRoutesPlugin and the route node with
routePath '/posts/$slug'), assert that globalThis.TSS_PRERENDER_DYNAMIC_ROUTES
does not contain an entry for '/posts/$slug' (or is empty), so the test checks
both TSS_PRERENDABLE_PATHS and globalThis.TSS_PRERENDER_DYNAMIC_ROUTES
consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b58420f7-00a8-4fa8-b77b-ab44168b7d58
📒 Files selected for processing (48)
.changeset/tall-trees-prerender-params.mddocs/start/framework/react/guide/static-prerendering.mddocs/start/framework/solid/guide/static-prerendering.mde2e/react-start/basic/src/routeTree.gen.tse2e/react-start/basic/src/routes/-prerender-params.server.tse2e/react-start/basic/src/routes/_layout/_layout-2/prerender-nested.$slug.tsxe2e/react-start/basic/src/routes/prerender-params.$slug.tsxe2e/react-start/basic/start-mode-config.tse2e/react-start/basic/tests/prerendering.spec.tse2e/solid-start/basic/package.jsone2e/solid-start/basic/rsbuild.config.tse2e/solid-start/basic/src/routeTree.gen.tse2e/solid-start/basic/src/routes/-prerender-params.server.tse2e/solid-start/basic/src/routes/_layout/_layout-2/prerender-nested.$slug.tsxe2e/solid-start/basic/src/routes/prerender-params.$slug.tsxe2e/solid-start/basic/tests/prerendering.spec.tse2e/solid-start/basic/vite.config.tse2e/vue-start/basic/package.jsone2e/vue-start/basic/rsbuild.config.tse2e/vue-start/basic/src/routeTree.gen.tse2e/vue-start/basic/src/routes/-prerender-params.server.tse2e/vue-start/basic/src/routes/_layout/_layout-2/prerender-nested.$slug.tsxe2e/vue-start/basic/src/routes/prerender-params.$slug.tsxe2e/vue-start/basic/tests/prerendering.spec.tse2e/vue-start/basic/vite.config.tspackages/start-client-core/src/index.tsxpackages/start-client-core/src/prerenderParams.tspackages/start-client-core/src/tests/prerenderParams.test-d.tspackages/start-plugin-core/src/build-sitemap.tspackages/start-plugin-core/src/global.d.tspackages/start-plugin-core/src/post-build.tspackages/start-plugin-core/src/prerender-params-runner.tspackages/start-plugin-core/src/prerender-route-options.tspackages/start-plugin-core/src/prerender.tspackages/start-plugin-core/src/rsbuild/post-build.tspackages/start-plugin-core/src/rsbuild/start-router-plugin.tspackages/start-plugin-core/src/schema.tspackages/start-plugin-core/src/start-router-plugin/constants.tspackages/start-plugin-core/src/start-router-plugin/generator-plugins/prerender-routes-plugin.tspackages/start-plugin-core/src/vite/prerender.tspackages/start-plugin-core/src/vite/start-router-plugin/plugin.tspackages/start-plugin-core/tests/build-sitemap.test.tspackages/start-plugin-core/tests/prerender-params-runner.test.tspackages/start-plugin-core/tests/prerender-routes-plugin.test.tspackages/start-plugin-core/tests/prerender-ssrf.test.tspackages/start-plugin-core/tests/start-router-plugin-constants.test.tspackages/start-server-core/src/createStartHandler.tspackages/start-server-core/src/global.d.ts
Merging this PR will not alter performance
Comparing 
Footnotes
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/start-plugin-core/src/prerender-params-runner.ts (2)
68-89: 💤 Low valueDefensive validation for non-iterable
entries.
for (const entry of entries)will throw a non-descriptiveTypeError: entries is not iterableif a user-authoredprerenderParamsmistakenly returnsundefined/nullor a non-array. A small guard with a clearer error message attributing the offending route improves DX without changing the happy path.♻️ Suggested guard
const entries = await call( () => options.prerenderParams!({ routePath: route.routePath, signal: controller.signal, }), controller.signal, ).finally(cleanupTimeout) + if (!Array.isArray(entries)) { + throw new Error( + `prerenderParams for route ${route.routePath} must return an array; got ${typeof entries}`, + ) + } + for (const entry of entries) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/start-plugin-core/src/prerender-params-runner.ts` around lines 68 - 89, The loop assumes the result of calling options.prerenderParams via call(...) is iterable; add a defensive validation after the await that checks entries is an array (or at least iterable) and throw a clear Error that includes route.routePath (or skip non-iterable with a logged warning) so users get a descriptive message instead of "entries is not iterable"; update the block around the awaited call in prerender-params-runner.ts (where entries is assigned from call(...), which uses controller.signal and options.prerenderParams) to validate entries before the for (const entry of entries) loop and only proceed to call create(...), filter, pagesByPath.set(...), and merge(...) when entries is valid.
155-186: 💤 Low valueRoute-level
options.prerenderis silently dropped on generated pages.
create()mergesoptions.sitemapwithentry.sitemapbut assignsprerender: entry.prerenderdirectly, ignoringoptions.prerenderentirely. If a route declaresprerender: { headers: { … } }at the route level and an entry omitsprerender, those headers are lost on the generated page even though the analogous sitemap config is preserved. If this asymmetry is intentional (e.g., prerender options are applied later in the pipeline), a brief comment would help; otherwise consider symmetric merging.♻️ Symmetric merge sketch
return { path: interpolatedPath + search(entry.search), sitemap: sitemap(options.sitemap, entry.sitemap), - prerender: entry.prerender, + prerender: prerender(options.prerender, entry.prerender), }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/start-plugin-core/src/prerender-params-runner.ts` around lines 155 - 186, The create function currently drops route-level prerender options by returning prerender: entry.prerender instead of merging with options.prerender; update create (in prerender-params-runner.ts) to merge options.prerender with entry.prerender (similar to how sitemap(options.sitemap, entry.sitemap) is merged) so route-level defaults (e.g., headers) are preserved when entry.prerender is absent—either call an existing merge helper or shallow/deep-merge options.prerender and entry.prerender and return the merged result under prerender.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/start-plugin-core/src/prerender-params-runner.ts`:
- Around line 68-89: The loop assumes the result of calling
options.prerenderParams via call(...) is iterable; add a defensive validation
after the await that checks entries is an array (or at least iterable) and throw
a clear Error that includes route.routePath (or skip non-iterable with a logged
warning) so users get a descriptive message instead of "entries is not
iterable"; update the block around the awaited call in
prerender-params-runner.ts (where entries is assigned from call(...), which uses
controller.signal and options.prerenderParams) to validate entries before the
for (const entry of entries) loop and only proceed to call create(...), filter,
pagesByPath.set(...), and merge(...) when entries is valid.
- Around line 155-186: The create function currently drops route-level prerender
options by returning prerender: entry.prerender instead of merging with
options.prerender; update create (in prerender-params-runner.ts) to merge
options.prerender with entry.prerender (similar to how sitemap(options.sitemap,
entry.sitemap) is merged) so route-level defaults (e.g., headers) are preserved
when entry.prerender is absent—either call an existing merge helper or
shallow/deep-merge options.prerender and entry.prerender and return the merged
result under prerender.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e8757849-427a-4490-af73-adf1d0583e55
📒 Files selected for processing (6)
e2e/react-start/basic/tests/prerendering.spec.tse2e/solid-start/basic/tests/prerendering.spec.tse2e/vue-start/basic/tests/prerendering.spec.tspackages/start-plugin-core/src/post-build.tspackages/start-plugin-core/src/prerender-params-runner.tspackages/start-plugin-core/tests/post-server-build.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- e2e/react-start/basic/tests/prerendering.spec.ts
- e2e/solid-start/basic/tests/prerendering.spec.ts
- e2e/vue-start/basic/tests/prerendering.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/start-plugin-core/src/post-build.ts (1)
17-18: 💤 Low value
spaOnlyis inferred asboolean | undefinedrather thanbooleanWhen
startConfig.spaisundefined, the?.enabledshort-circuits the&&toundefined(notfalse). All current usages (if (spaOnly), ternary) treat it as a falsy check, so there's no runtime defect, but the inferred typeboolean | undefinedis weaker than needed under strict mode.♻️ Suggested refinement
- const spaOnly = - startConfig.spa?.enabled && startConfig.prerender?.enabled !== true + const spaOnly = + !!startConfig.spa?.enabled && startConfig.prerender?.enabled !== true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/start-plugin-core/src/post-build.ts` around lines 17 - 18, The variable spaOnly is inferred as boolean | undefined because startConfig.spa?.enabled can short-circuit to undefined; coerce the result to a true boolean so its type is boolean (e.g., wrap the whole expression with a boolean coercion such as Boolean(...) or double-negation) and keep the same logical check using startConfig.spa?.enabled and startConfig.prerender?.enabled !== true so usages like if (spaOnly) remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/start-plugin-core/tests/post-server-build.test.ts`:
- Around line 14-41: The test is misleading because postBuild does not read
globalThis.TSS_PRERENDER_DYNAMIC_ROUTES; remove the global setup and adjust the
test to assert the actual condition under test by deleting the
globalThis.TSS_PRERENDER_DYNAMIC_ROUTES assignment (and its cleanup) and rename
the it(...) description to something like "does not enable prerendering when
pages array is empty and prerender config is absent" while keeping the same call
to postBuild, the mocked adapter.prerender and the expectation that prerender
was not called; alternatively move this scenario into an integration test that
invokes the real prerender implementation if you intend to assert behavior
around TSS_PRERENDER_DYNAMIC_ROUTES.
---
Nitpick comments:
In `@packages/start-plugin-core/src/post-build.ts`:
- Around line 17-18: The variable spaOnly is inferred as boolean | undefined
because startConfig.spa?.enabled can short-circuit to undefined; coerce the
result to a true boolean so its type is boolean (e.g., wrap the whole expression
with a boolean coercion such as Boolean(...) or double-negation) and keep the
same logical check using startConfig.spa?.enabled and
startConfig.prerender?.enabled !== true so usages like if (spaOnly) remain
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3acc396d-cd42-4fb7-9e36-8cac7d3d593d
📒 Files selected for processing (4)
packages/start-plugin-core/src/post-build.tspackages/start-plugin-core/src/prerender.tspackages/start-plugin-core/src/vite/prerender.tspackages/start-plugin-core/tests/post-server-build.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/start-plugin-core/src/vite/prerender.ts
- packages/start-plugin-core/src/prerender.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Nx Cloud has identified a flaky task in your failed CI:
Since the failure was identified as flaky, the solution is to rerun CI. Because this branch comes from a fork, it is not possible for us to push directly, but you can rerun by pushing an empty commit:
git commit --allow-empty -m "chore: trigger rerun"
git push
🎓 Learn more about Self-Healing CI on nx.dev
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (3)
e2e/solid-start/basic/rsbuild.config.ts (1)
7-23: ⚡ Quick winType-check prerender/sitemap config objects against plugin option contracts.
Given this is TypeScript config, bind these objects with
satisfiesto thetanstackStartoption types so API drift is caught at compile time.Suggested patch
+type StartOptions = NonNullable<Parameters<typeof tanstackStart>[0]> + const prerenderConfiguration = { enabled: true, filter: (page: { path: string }) => @@ - maxRedirects: 100, -} + maxRedirects: 100, +} satisfies NonNullable<StartOptions['prerender']> @@ tanstackStart({ prerender: isPrerender ? prerenderConfiguration : undefined, sitemap: isPrerender - ? { + ? ({ enabled: true, host: 'https://example.com', - } + } satisfies NonNullable<StartOptions['sitemap']>) : undefined, }),As per coding guidelines,
**/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety.Also applies to: 35-40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/solid-start/basic/rsbuild.config.ts` around lines 7 - 23, The prerenderConfiguration object is not type-checked against the plugin option contract; update it to use TypeScript's "satisfies" operator to bind it to the appropriate tanstackStart option type (the same contract used for plugin options) so API drift is caught at compile time — locate the prerenderConfiguration constant and any similar config objects (also around the other config block at lines ~35-40) and change their declarations to append "satisfies <TanstackStartOptionsType>" (use the exact exported type name from the tanstackStart plugin) so the compiler enforces the expected shape while keeping the inferred narrower types.packages/start-plugin-core/tests/build-sitemap.test.ts (1)
35-35: 💤 Low valueRepeated
as anycasts reduce type-safety coverage of the testsEvery test passes
startConfigasas any(lines 35, 74, 105, 131, 156, 177, 221), which silently allows structurally invalid input and defeats the TypeScript-strict-mode requirement for these tests.Consider exporting a minimal
DeepPartial<BuildSitemapOptions>or a dedicated test-fixture type so the test inputs remain typed. If the config type is intentionally unexported, at minimum useas unknown as BuildSitemapOptionsto at least require the cast to be explicit. As per coding guidelines,.tsfiles should use TypeScript strict mode with extensive type safety.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/start-plugin-core/tests/build-sitemap.test.ts` at line 35, Replace the repeated unsafe "as any" casts for test inputs with a typed test fixture: define a minimal fixture type (e.g., DeepPartial<BuildSitemapOptions> or a TestBuildSitemapOptions) and update every startConfig usage (the objects currently cast with "as any") to be typed as that fixture or cast as "as unknown as BuildSitemapOptions" if the real type is unexported; change the seven occurrences (the casts at startConfig in build-sitemap.test.ts) so tests maintain TS strict-mode safety and avoid blanket "as any" casting while still allowing partial test objects.packages/start-plugin-core/src/rsbuild/virtual-modules.ts (1)
569-577: 💤 Low valueConsider mirroring the environment loop from
updateServerFnResolverintryUpdateServerFnResolverfor consistency.
updateServerFnResolver(lines 550–567) iterates through server, prerender (if registered), and provider environments, whiletryUpdateServerFnResolveronly updates the server environment. If a prerender VMP is registered, the prerender resolver will silently diverge. Though the explicit-content call site is server-only today, mirroring the loop would prevent future issues and maintain consistency between the two methods.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/start-plugin-core/src/rsbuild/virtual-modules.ts` around lines 569 - 577, tryUpdateServerFnResolver currently only updates the server environment, which can diverge from updateServerFnResolver that loops server → prerender (if a prerender VMP is registered) → provider; change tryUpdateServerFnResolver to mirror updateServerFnResolver by iterating the same environments and updating lastResolverContentByEnvironment and calling tryWriteModule for each environment’s resolver path (e.g., paths.serverFnResolver, paths.prerenderFnResolver, paths.providerFnResolver), including the conditional check used in updateServerFnResolver for whether the prerender VMP is registered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/start/framework/solid/guide/static-prerendering.md`:
- Around line 78-79: Update the note explaining the default separate
route-options bundling to explicitly call out the Nitro adapter exception: state
that while Start defaults to building route options separately
(prerender.separateRouteOptionsBundle = true), Nitro-based deployments/adapter
keep this setting off for compatibility, so Nitro users should set or expect
prerender.separateRouteOptionsBundle = false; modify the sentence around the
existing mention of prerender.separateRouteOptionsBundle to include this
Nitro-specific caveat and suggested wording for clarity.
In `@e2e/react-start/basic/tests/prerendering.spec.ts`:
- Around line 28-35: The marker-checking helper outputContainsMarker only looks
at '.js' files and thus misses '.mjs' bundles; update the check in function
outputContainsMarker (and the predicate that currently uses
filePath.endsWith('.js')) to also include '.mjs' (e.g., accept files that end
with '.js' or '.mjs') so prerender-only markers in .mjs server output are
detected during the assertions.
In `@e2e/solid-start/basic/rsbuild.config.ts`:
- Around line 9-22: The prerender filter uses substring matching via
page.path.includes(p) which can wrongly exclude routes like "/usersettings" when
"/users" is in the list; update the filter (the filter function that references
page.path.includes) to perform exact-or-boundary matching instead (e.g., compare
full path equality or use a path-boundary regex or check segment boundaries) so
only exact route matches or path-segment matches are excluded; keep the
exclusion list (the array of p values) and replace the includes check with a
robust equality/boundary check that treats "/users" as a full segment (e.g.,
equals or regex matching for end-of-string or slash) to avoid false positives.
In `@e2e/vue-start/basic/rsbuild.config.ts`:
- Around line 10-22: The filter predicate currently uses page.path.includes(p)
which accidentally excludes any route containing strings like '/users' or
'/search-params' (e.g., '/users/1'); update the check in the filter (the
function using page: { path: string } and the array of excluded strings) to
perform strict matching instead of substring matching — for example use
page.path === p for exact entries and/or page.path.startsWith(p + '/') for
prefix exclusions — and adjust the exclusion array entries ('/users',
'/search-params', etc.) accordingly so only intended routes are skipped.
In `@packages/start-plugin-core/src/post-build.ts`:
- Around line 26-28: The expression computing spaOnly uses
startConfig.prerender.enabled without optional chaining which can fail
TypeScript strict checks; update the references to use optional chaining (e.g.,
startConfig.prerender?.enabled) wherever prerender.enabled is read (including
the spaOnly assignment and the later check around prerender use) so the compiler
knows the property access is guarded while preserving the same boolean logic
involving startConfig.spa?.enabled and prerender enabled state.
In `@packages/start-plugin-core/src/prerender-params-runner.ts`:
- Around line 201-205: The generated page object is discarding route-level
prerender defaults by assigning entry.prerender directly; change the return to
merge route-level and entry-level prerender so entry values override defaults
(similar to how sitemap is merged). Locate the return that currently sets
prerender: entry.prerender and replace it with a merged value, e.g. use
mergeOptions(options.prerender, entry.prerender) (or the existing merge helper
used for sitemap) so dynamic pages inherit route defaults while allowing
per-entry overrides.
In `@packages/start-plugin-core/src/prerender.ts`:
- Around line 64-70: The code is double-normalizing prerender paths: remove the
extra call to validateAndNormalizePrerenderPages so paths are normalized only
once by prerenderPages; specifically, stop reassigning startConfig.pages here
(the block that builds routerBasePath/routerBaseUrl and calls
validateAndNormalizePrerenderPages) because prerenderPages already invokes
validateAndNormalizePrerenderPages after runPrerenderParams—leave
routerBasePath/routerBaseUrl usage if needed but eliminate the duplicate
normalize step to avoid decoding percent-encoded content twice (see
validateAndNormalizePrerenderPages, prerenderPages, runPrerenderParams, and
startConfig.pages).
In `@packages/start-plugin-core/src/rsbuild/start-compiler-host.ts`:
- Around line 80-83: The default environments array assigned to environments
(from opts.environments) omits the prerender environment while later code checks
for RSBUILD_ENVIRONMENT_NAMES.prerender; update the fallback so environments
defaults to include { name: RSBUILD_ENVIRONMENT_NAMES.prerender, type:
'prerender' } alongside client and server to ensure the prerender branch in
startCompilerHost (and any handling that checks for
RSBUILD_ENVIRONMENT_NAMES.prerender) can run when opts.environments is omitted.
In `@packages/start-plugin-core/tests/build-sitemap.test.ts`:
- Around line 114-146: The test sets process.env.SITE_URL but still passes host:
process.env.SITE_URL into buildSitemap, so it never exercises the env-fallback;
to fix, remove the explicit host property from the startConfig.sitemap object
passed to buildSitemap so the function (buildSitemap) will read
process.env.SITE_URL at runtime and the existing try/finally env save/restore
remains meaningful; update the test expectation to remain the same and keep temp
dir setup and cleanup as-is.
In `@packages/start-plugin-core/tests/prerender-params-runner.test.ts`:
- Around line 334-339: The exact string assertion for the AbortController
rejection is brittle across runtimes; update the test that constructs
expectation from result (the line using expect(result).rejects.toThrow(...)) to
assert against a regex like /operation was aborted/i instead of the exact string
so it matches both Node and Bun; keep the existing process.emit('SIGTERM') and
await expectation flow but replace the literal message in the toThrow call
(refer to the expect(result).rejects.toThrow usage and the expectation variable)
with a case-insensitive regex matching "operation was aborted".
In `@packages/start-plugin-core/tests/rsbuild-post-build.test.ts`:
- Around line 13-18: The beforeEach setup currently resets modules and clears
__ROUTE_OPTIONS_LOADED and some env vars but doesn't clear the mutated global
route tree; update the beforeEach to also delete or reset
globalThis.TSS_PRERENDER_ROUTE_TREE so each test starts with a clean
state—locate the beforeEach block and add removal of
globalThis.TSS_PRERENDER_ROUTE_TREE (or set it to undefined/null) alongside the
existing deletes.
---
Nitpick comments:
In `@e2e/solid-start/basic/rsbuild.config.ts`:
- Around line 7-23: The prerenderConfiguration object is not type-checked
against the plugin option contract; update it to use TypeScript's "satisfies"
operator to bind it to the appropriate tanstackStart option type (the same
contract used for plugin options) so API drift is caught at compile time —
locate the prerenderConfiguration constant and any similar config objects (also
around the other config block at lines ~35-40) and change their declarations to
append "satisfies <TanstackStartOptionsType>" (use the exact exported type name
from the tanstackStart plugin) so the compiler enforces the expected shape while
keeping the inferred narrower types.
In `@packages/start-plugin-core/src/rsbuild/virtual-modules.ts`:
- Around line 569-577: tryUpdateServerFnResolver currently only updates the
server environment, which can diverge from updateServerFnResolver that loops
server → prerender (if a prerender VMP is registered) → provider; change
tryUpdateServerFnResolver to mirror updateServerFnResolver by iterating the same
environments and updating lastResolverContentByEnvironment and calling
tryWriteModule for each environment’s resolver path (e.g.,
paths.serverFnResolver, paths.prerenderFnResolver, paths.providerFnResolver),
including the conditional check used in updateServerFnResolver for whether the
prerender VMP is registered.
In `@packages/start-plugin-core/tests/build-sitemap.test.ts`:
- Line 35: Replace the repeated unsafe "as any" casts for test inputs with a
typed test fixture: define a minimal fixture type (e.g.,
DeepPartial<BuildSitemapOptions> or a TestBuildSitemapOptions) and update every
startConfig usage (the objects currently cast with "as any") to be typed as that
fixture or cast as "as unknown as BuildSitemapOptions" if the real type is
unexported; change the seven occurrences (the casts at startConfig in
build-sitemap.test.ts) so tests maintain TS strict-mode safety and avoid blanket
"as any" casting while still allowing partial test objects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff844a3b-c3ce-4c23-b7bf-144598cfcc65
📒 Files selected for processing (37)
docs/start/framework/react/guide/static-prerendering.mddocs/start/framework/solid/guide/static-prerendering.mde2e/react-start/basic/src/routes/prerender-params.$slug.tsxe2e/react-start/basic/tests/prerendering.spec.tse2e/solid-start/basic/rsbuild.config.tse2e/vue-start/basic/rsbuild.config.tspackages/start-plugin-core/src/constants.tspackages/start-plugin-core/src/global.d.tspackages/start-plugin-core/src/post-build.tspackages/start-plugin-core/src/prerender-params-runner.tspackages/start-plugin-core/src/prerender-route-options-env.tspackages/start-plugin-core/src/prerender.tspackages/start-plugin-core/src/rsbuild/planning.tspackages/start-plugin-core/src/rsbuild/plugin.tspackages/start-plugin-core/src/rsbuild/post-build.tspackages/start-plugin-core/src/rsbuild/start-compiler-host.tspackages/start-plugin-core/src/rsbuild/start-router-plugin.tspackages/start-plugin-core/src/rsbuild/types.tspackages/start-plugin-core/src/rsbuild/virtual-modules.tspackages/start-plugin-core/src/schema.tspackages/start-plugin-core/src/start-router-plugin/constants.tspackages/start-plugin-core/src/vite/planning.tspackages/start-plugin-core/src/vite/plugin.tspackages/start-plugin-core/src/vite/prerender.tspackages/start-plugin-core/src/vite/start-compiler-plugin/plugin.tspackages/start-plugin-core/src/vite/start-router-plugin/plugin.tspackages/start-plugin-core/tests/build-sitemap.test.tspackages/start-plugin-core/tests/post-server-build.test.tspackages/start-plugin-core/tests/prerender-params-runner.test.tspackages/start-plugin-core/tests/prerender-route-options-env.test.tspackages/start-plugin-core/tests/prerender-routes-plugin.test.tspackages/start-plugin-core/tests/prerender-ssrf.test.tspackages/start-plugin-core/tests/rsbuild-post-build.test.tspackages/start-plugin-core/tests/start-router-plugin-constants.test.tspackages/start-plugin-core/tests/vite-planning.test.tspackages/start-plugin-core/tests/vite-prerender.test.tspackages/start-server-core/src/createStartHandler.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (9)
packages/start-client-core/src/tests/prerenderParams.test-d.ts (1)
13-227: ⚡ Quick winAdd type tests for async
prerenderParamsand per-entrysitemapmetadata.This suite currently locks sync callback typing and route-level sitemap typing, but not the async callback form or
entry.sitemaptyping on returned entries. Adding oneasync prerenderParamscase and one entry-level sitemap case would better protect the new API surface from type regressions.As per coding guidelines, "Use TypeScript strict mode with extensive type safety."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/start-client-core/src/tests/prerenderParams.test-d.ts` around lines 13 - 227, Add two type tests: one for an async prerenderParams function and one asserting per-entry sitemap typing. Create an options object whose prerenderParams is async (async () => [...]) and use it with FileBaseRouteOptions to derive Entry = Awaited<ReturnType<NonNullable<typeof options.prerenderParams>>>[number], then expectTypeOf<Entry['params']>() and expectTypeOf<Entry['sitemap']>() to match expected shapes (e.g., priority: number, changefreq: 'weekly' | ...). Also add a returned entry that includes an entry-level sitemap field to ensure per-entry sitemap is type-checked against the route-level sitemap shape.packages/start-plugin-core/tests/build-sitemap.test.ts (1)
147-212: 💤 Low value
writes advanced sitemap metadatadoes not assert the<loc>URL.All namespace, image, news, and alternate-ref assertions are present, but no assertion verifies the
<loc>element for/news/router-launch. IfbuildSitemapever emits a malformed URL for this entry the test would not catch it.✅ Suggested addition
const sitemap = readFileSync(join(publicDir, 'sitemap.xml'), 'utf-8') + expect(sitemap).toContain( + '<loc>https://example.com/news/router-launch</loc>', + ) expect(sitemap).toContain( 'xmlns:image="http://www.google.com/schemas/sitemap-image/1.1"', )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/start-plugin-core/tests/build-sitemap.test.ts` around lines 147 - 212, The test "writes advanced sitemap metadata" for buildSitemap is missing an assertion for the <loc> element; update the test in build-sitemap.test.ts (inside the it block that calls buildSitemap and reads sitemap into the sitemap variable) to assert that the sitemap contains the canonical URL for the page path (e.g. expect(sitemap).toContain('<loc>https://example.com/news/router-launch</loc>') so the produced <loc> element for the /news/router-launch entry is validated).e2e/react-start/basic/tests/prerendering.spec.ts (2)
99-99: ⚡ Quick winAvoid asserting React’s empty-comment serialization.
These checks are tied to the current
<!-- -->HTML shape, so they can fail on harmless SSR serialization changes even when the prerendered content is still correct. Matching the text with the same regex style used on Line 141 and Line 142 would make this coverage much less brittle.♻️ Suggested change
- expect(html).toContain('Nested prerendered slug: <!-- -->under-layout') + expect(html).toMatch( + /Nested prerendered slug:(?:\s|<!--[^>]*-->)*under-layout/, + ) - expect(html).toContain('Prerendered slug: <!-- -->hello-world') + expect(html).toMatch( + /Prerendered slug:(?:\s|<!--[^>]*-->)*hello-world/, + )Also applies to: 108-108
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/react-start/basic/tests/prerendering.spec.ts` at line 99, The assertion in prerendering.spec.ts that expects the literal string "Nested prerendered slug: <!-- -->under-layout" is brittle because it depends on React's empty-comment HTML serialization; replace that expect(html).toContain(...) check with a regex-based assertion (as used on the other assertions around Line 141/142) that matches the visible text regardless of the empty-comment node (e.g., assert that html matches /Nested prerendered slug:\s*under-layout/), and apply the same change for the similar assertion at the other occurrence (the expect call containing the same comment-based string).
203-216: ⚡ Quick winScope sitemap metadata assertions to the matching
<url>entry.As written, this can still pass if
lastmod/priority/changefreqend up on the wrong sitemap entry, because each value is asserted independently. Since this PR adds per-entry sitemap overrides, it would be safer to assert against the full<url>block for each route or parse the XML and check the fields on the matching<loc>.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/react-start/basic/tests/prerendering.spec.ts` around lines 203 - 216, The test currently asserts sitemap metadata values independently which can misattribute values to the wrong <url>; update the assertions to scope them to the matching <url> block by locating the <url> node for each <loc> and then asserting its child tags (e.g., <lastmod>, <priority>, <changefreq>) match expected values. In the prerendering.spec.ts test use the sitemap string (sitemap) and either parse the XML (e.g., a lightweight XML parser) to find the <url> element whose <loc> equals the target URL and assert its children, or assert against the full expected <url> block string for each route (e.g., the block containing '<loc>https://example.com/prerender-params/hello-world</loc>') so lastmod/priority/changefreq are validated within the same <url>.packages/start-plugin-core/tests/prerender-params-runner.test.ts (1)
4-6: 💤 Low valueConsider resetting the shared logger spy between tests.
The
logger.warnspy persists across tests; only the static-route test (line 456) explicitly callsmockClear(). If future tests addtoHaveBeenCalledWithor call-count assertions, residue from earlier tests could cause flaky behavior. AbeforeEach(() => logger.warn.mockClear())would make tests independent of order.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/start-plugin-core/tests/prerender-params-runner.test.ts` around lines 4 - 6, Add a beforeEach hook to clear the shared logger spy so tests don't leak calls: call logger.warn.mockClear() (or vi.clearAllMocks()) in a beforeEach at the top of the test suite that contains the logger definition so logger.warn is reset before each test; this targets the logger.warn spy used in the tests (and keeps the static-route test's explicit mockClear safe to keep).packages/start-plugin-core/src/prerender-params-runner.ts (3)
110-120: 💤 Low valueProcess signal handlers will swallow the first SIGINT/SIGTERM during prerender.
process.onceensures the handler fires only on the first signal - subsequent signals revert to default (process termination). However, whilerunPrerenderParamsis running, the first SIGINT/SIGTERM aborts the prerenderParams controller but does NOT terminate the process. This is likely intentional (graceful abort), but it means a single Ctrl+C will be effectively swallowed: the prerenderParams call rejects with the abort reason, then the surroundingprerender()flow continues with that error. Users may need to press Ctrl+C twice. Consider documenting this behavior or re-emitting the signal after abort if termination is the intent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/start-plugin-core/src/prerender-params-runner.ts` around lines 110 - 120, The current attachProcessAbortHandlers function uses process.once('SIGINT'/'SIGTERM') to call controller.abort(), which aborts the prerender controller but swallows the first signal (requiring a second Ctrl+C to terminate); update attachProcessAbortHandlers (or the call site around runPrerenderParams/prerender) to either re-emit the original signal after calling controller.abort() (e.g., restore default behavior or call process.kill(process.pid, signal) so the process will terminate on the first signal) or explicitly document this single-signal-graceful-abort behavior in the prerender/runPrerenderParams docs so users know they must press Ctrl+C twice — locate attachProcessAbortHandlers, the abort callback that calls controller.abort(), and the runPrerenderParams/prerender flow to implement the chosen change.
231-238: 💤 Low value
mergeOptionsis shallow - nested sitemap fields will be replaced wholesale.
mergeOptionsonly does a shallow spread, which is correct for top-level keys but means nested objects/arrays (e.g.sitemap.images,sitemap.news,sitemap.alternateRefs) are replaced rather than merged when both base and override define them. This appears intentional given the test expectations, but worth a brief docstring noting the behavior so future contributors don't assume deep-merge semantics for nested arrays like alternateRefs/images.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/start-plugin-core/src/prerender-params-runner.ts` around lines 231 - 238, The mergeOptions function currently performs a shallow merge for T extends RouteSitemapOptions | RoutePrerenderOptions so nested sitemap fields (e.g. sitemap.images, sitemap.news, sitemap.alternateRefs) in override will replace those in base rather than being deep-merged; add a concise docstring above mergeOptions that states this behavior explicitly (shallow merge only, nested objects/arrays are replaced), mention the relevant types (RouteSitemapOptions, RoutePrerenderOptions) and the specific nested keys, and optionally note that tests expect this semantics so future contributors won't assume deep-merge behavior.
240-242: 💤 Low value
isDynamicPathcheck aligns with TanStack Router's own pattern detection.The
path.includes('$')check mirrors TanStack Router's own approach ininterpolatePath(router-core/src/path.ts:260), which uses the same quick filter before detailedparseSegmentparsing for complex patterns like$param,{-$optional}, and{$}wildcards. While a more precise regex could be used, the current implementation is pragmatic and consistent with how TanStack Router itself detects dynamic paths. The risk of misclassifying literal$in developer-defined route patterns is minimal since URL paths rarely contain unencoded$characters.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/start-plugin-core/src/prerender-params-runner.ts` around lines 240 - 242, The current isDynamicPath function uses a simple dollar-sign presence check (function isDynamicPath(path: string) { return path.includes('$') }) which matches TanStack Router's pragmatic quick-filter behavior; leave this implementation unchanged to remain consistent with TanStack Router's intercept in interpolatePath and avoid overcomplicating with a stricter regex unless you need to handle literal unencoded '$' in paths—so keep isDynamicPath as-is (no code change).packages/start-plugin-core/src/prerender-route-options.ts (1)
72-82: 💤 Low valueDefensive children handling looks good, but consider documenting the assumption.
The code handles
childrenas either an array or an object (viaObject.values). In current TanStack Router,route.childrenis an array, but supporting both forms is defensive and harmless. A brief comment noting why both shapes are handled (or any router-core variant that returns the object form) would help future readers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/start-plugin-core/src/prerender-route-options.ts` around lines 72 - 82, The children-handling branch in prerender-route-options.ts (the const children = route.children check and the for loop over Array.isArray(children) ? children : Object.values(children)) should include a short inline comment explaining the assumption: TanStack Router currently supplies an array but some router-core variants or legacy shapes may expose children as an object, so we defensively support both forms; reference this rationale above the children variable and mention AnyRoute/visit usage so future readers understand why we call Object.values for the object shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@e2e/react-start/basic/tests/prerendering.spec.ts`:
- Line 99: The assertion in prerendering.spec.ts that expects the literal string
"Nested prerendered slug: <!-- -->under-layout" is brittle because it depends on
React's empty-comment HTML serialization; replace that
expect(html).toContain(...) check with a regex-based assertion (as used on the
other assertions around Line 141/142) that matches the visible text regardless
of the empty-comment node (e.g., assert that html matches /Nested prerendered
slug:\s*under-layout/), and apply the same change for the similar assertion at
the other occurrence (the expect call containing the same comment-based string).
- Around line 203-216: The test currently asserts sitemap metadata values
independently which can misattribute values to the wrong <url>; update the
assertions to scope them to the matching <url> block by locating the <url> node
for each <loc> and then asserting its child tags (e.g., <lastmod>, <priority>,
<changefreq>) match expected values. In the prerendering.spec.ts test use the
sitemap string (sitemap) and either parse the XML (e.g., a lightweight XML
parser) to find the <url> element whose <loc> equals the target URL and assert
its children, or assert against the full expected <url> block string for each
route (e.g., the block containing
'<loc>https://example.com/prerender-params/hello-world</loc>') so
lastmod/priority/changefreq are validated within the same <url>.
In `@packages/start-client-core/src/tests/prerenderParams.test-d.ts`:
- Around line 13-227: Add two type tests: one for an async prerenderParams
function and one asserting per-entry sitemap typing. Create an options object
whose prerenderParams is async (async () => [...]) and use it with
FileBaseRouteOptions to derive Entry = Awaited<ReturnType<NonNullable<typeof
options.prerenderParams>>>[number], then expectTypeOf<Entry['params']>() and
expectTypeOf<Entry['sitemap']>() to match expected shapes (e.g., priority:
number, changefreq: 'weekly' | ...). Also add a returned entry that includes an
entry-level sitemap field to ensure per-entry sitemap is type-checked against
the route-level sitemap shape.
In `@packages/start-plugin-core/src/prerender-params-runner.ts`:
- Around line 110-120: The current attachProcessAbortHandlers function uses
process.once('SIGINT'/'SIGTERM') to call controller.abort(), which aborts the
prerender controller but swallows the first signal (requiring a second Ctrl+C to
terminate); update attachProcessAbortHandlers (or the call site around
runPrerenderParams/prerender) to either re-emit the original signal after
calling controller.abort() (e.g., restore default behavior or call
process.kill(process.pid, signal) so the process will terminate on the first
signal) or explicitly document this single-signal-graceful-abort behavior in the
prerender/runPrerenderParams docs so users know they must press Ctrl+C twice —
locate attachProcessAbortHandlers, the abort callback that calls
controller.abort(), and the runPrerenderParams/prerender flow to implement the
chosen change.
- Around line 231-238: The mergeOptions function currently performs a shallow
merge for T extends RouteSitemapOptions | RoutePrerenderOptions so nested
sitemap fields (e.g. sitemap.images, sitemap.news, sitemap.alternateRefs) in
override will replace those in base rather than being deep-merged; add a concise
docstring above mergeOptions that states this behavior explicitly (shallow merge
only, nested objects/arrays are replaced), mention the relevant types
(RouteSitemapOptions, RoutePrerenderOptions) and the specific nested keys, and
optionally note that tests expect this semantics so future contributors won't
assume deep-merge behavior.
- Around line 240-242: The current isDynamicPath function uses a simple
dollar-sign presence check (function isDynamicPath(path: string) { return
path.includes('$') }) which matches TanStack Router's pragmatic quick-filter
behavior; leave this implementation unchanged to remain consistent with TanStack
Router's intercept in interpolatePath and avoid overcomplicating with a stricter
regex unless you need to handle literal unencoded '$' in paths—so keep
isDynamicPath as-is (no code change).
In `@packages/start-plugin-core/src/prerender-route-options.ts`:
- Around line 72-82: The children-handling branch in prerender-route-options.ts
(the const children = route.children check and the for loop over
Array.isArray(children) ? children : Object.values(children)) should include a
short inline comment explaining the assumption: TanStack Router currently
supplies an array but some router-core variants or legacy shapes may expose
children as an object, so we defensively support both forms; reference this
rationale above the children variable and mention AnyRoute/visit usage so future
readers understand why we call Object.values for the object shape.
In `@packages/start-plugin-core/tests/build-sitemap.test.ts`:
- Around line 147-212: The test "writes advanced sitemap metadata" for
buildSitemap is missing an assertion for the <loc> element; update the test in
build-sitemap.test.ts (inside the it block that calls buildSitemap and reads
sitemap into the sitemap variable) to assert that the sitemap contains the
canonical URL for the page path (e.g.
expect(sitemap).toContain('<loc>https://example.com/news/router-launch</loc>')
so the produced <loc> element for the /news/router-launch entry is validated).
In `@packages/start-plugin-core/tests/prerender-params-runner.test.ts`:
- Around line 4-6: Add a beforeEach hook to clear the shared logger spy so tests
don't leak calls: call logger.warn.mockClear() (or vi.clearAllMocks()) in a
beforeEach at the top of the test suite that contains the logger definition so
logger.warn is reset before each test; this targets the logger.warn spy used in
the tests (and keeps the static-route test's explicit mockClear safe to keep).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea025d1a-1719-4e46-86c7-006c79ff430a
📒 Files selected for processing (12)
e2e/react-start/basic/tests/prerendering.spec.tse2e/solid-start/basic/rsbuild.config.tse2e/vue-start/basic/rsbuild.config.tspackages/start-client-core/src/tests/prerenderParams.test-d.tspackages/start-plugin-core/src/post-build.tspackages/start-plugin-core/src/prerender-params-runner.tspackages/start-plugin-core/src/prerender-route-options.tspackages/start-plugin-core/src/prerender.tspackages/start-plugin-core/src/rsbuild/start-compiler-host.tspackages/start-plugin-core/tests/build-sitemap.test.tspackages/start-plugin-core/tests/prerender-params-runner.test.tspackages/start-plugin-core/tests/rsbuild-post-build.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds route-level
prerenderParamssupport for TanStack Start so dynamic routes can declare which params/search combinations should be prerendered at build time.This lets routes like
/posts/$postIdgenerate static output for known params without manually listing every final URL in the global prerender config. It also makes dynamic prerendering first-class for sitemap generation, sositemap.xmlcan include URLs produced from route params instead of only statically discovered routes.Features
prerenderParamsroute options through Start route module augmentation.prerenderParamscallbacks with abort signal support.paramsand optionalsearchvalues.sitemapmetadata for generated dynamic pages.sitemap.xmlentries for dynamic URLs produced byprerenderParams.priority,changefreq,lastmod, images, news, and alternate refs.sitemapandprerenderoverrides.prerender.prerenderParamsTimeoutfor build-time callback timeout control.prerender.separateRouteOptionsBundleto control whether prerender-only route options are built separately from the final server bundle.Bundle Behavior
prerenderParamsandsitemaproute options from client bundles.prerender.separateRouteOptionsBundle: falsewhen users prefer fewer build steps or when an adapter cannot support the extra environment.Coverage
ssr: false/ selective SSR fixtures.sitemap.xmlgeneration from static routes, route-level sitemap metadata, and dynamicprerenderParamsentries.Notes On Adapter Coverage
Cloudflare, Netlify, and Nitro builds were smoke-tested locally.
Bun was not covered locally in this pass. The PR keeps
separateRouteOptionsBundleconfigurable so deployment adapters can opt out of the extra prerender route-options environment if needed, and Nitro already does this by default for compatibility.Docs
prerenderParams.sitemap.xmlgeneration for dynamic prerendered URLs.prerenderParamsTimeout.separateRouteOptionsBundle, including the Nitro compatibility behavior.Summary by CodeRabbit
New Features
Documentation
Tests